gh-91049: Introduce set vectorcall field API for PyFunctionObject #92257

adphrost · 2022-05-03T19:33:02Z

Avoid specializing functions with overridden vectorcall field

Issue: Allow extensions to set the vectorcall field on instances of PyFunctionObject #91049

bedevere-bot · 2022-05-03T19:33:05Z

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.

adphrost · 2022-05-03T19:39:26Z

@markshannon Can you let me know if there should be any other checks in ceval.c besides the one added in this PR?

For context, this check fixes an edge case where calling function with a set vectorfield only worked with a unpacked tuple, e.g. f(1) failed but f(*(1,)) succeeded (now both succeed)

itamaro

thanks for working on this @adphrost !

Lib/test/test_call.py

itamaro · 2022-05-03T19:47:42Z

Python/ceval.c

@@ -4792,7 +4792,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
            PyObject *function = PEEK(total_args + 1);
            int positional_args = total_args - KWNAMES_LEN();
            // Check if the call can be inlined or not
-            if (Py_TYPE(function) == &PyFunction_Type && tstate->interp->eval_frame == NULL) {
+            if (Py_TYPE(function) == &PyFunction_Type && tstate->interp->eval_frame == NULL && ((PyFunctionObject *)function)->vectorcall == _PyFunction_Vectorcall) {


this probably exceeds line length

What is the max line length allowed? And is there a linter I can run to handle this for me?

vstinner · 2022-05-03T19:50:15Z

Misc/NEWS.d/next/C API/2022-05-03-19-35-37.gh-issue-92193.61VoFL.rst

+
+This PR introduces an API call where vectorcall field can be modified like so:
+
+`void PyFunction_SetVectorcall(PyFunctionObject *func, vectorcallfunc vectorcall);`


Please document the function in Doc/c-api/function.rst and in the NEWS entry, refer to use with:

:c:func:`PyFunction_SetVectorcall`

New public functions should also be documented in What's New in Python 3.11 > C API > New features: Doc/whatsnew/3.11.rst.

markshannon

A few comments.
Have you attempted to measure the performance impact of this? (I'd expect it to be small or negligible).

Objects/funcobject.c

Modules/_testcapimodule.c

Change how assertion is done Co-authored-by: Itamar Ostricher <itamarost@gmail.com>

cpython-cla-bot · 2022-05-03T21:00:13Z

All commit authors signed the Contributor License Agreement.

itamaro · 2022-05-04T19:28:59Z

Have you attempted to measure the performance impact of this? (I'd expect it to be small or negligible).

@markshannon is there a simple way to ask a buildbot to run pyperformance comparing this against the base revision? we can do that manually, but a buildbot doing it for us would be nice :)

itamaro · 2022-05-19T03:43:10Z

circling back to @markshannon 's performance impact question. -- tldr Geometric mean: 1.00x faster

some more details:

rebased this PR onto a4460f2 and ran pyperformance on a4460f2 and on the rebased branch

both baseline and branch were built using ./configure --enable-optimizations --with-lto && make -j

used pyperformance 1.0.5 (installed directly from GitHub) on bare metal AWS machine (Linux-5.13.0-1023-aws-x86_64-with-glibc2.31 with 72 logical CPUs), and excluded the sqlalchemy benchmarks because they tried building greenlet and failed

pyperformance run --benchmarks=-sqlalchemy_declarative,-sqlalchemy_imperative --python builds/3.12-opt/python -o main.json --rigorous
pyperformance run --benchmarks=-sqlalchemy_declarative,-sqlalchemy_imperative --python builds/3.12-gh-91049-vectorcall-opt/python -o gh-91049.json --rigorous

compare results:

pyperf compare_to main.json gh-91049.json -G
Slower (20):
- regex_effbot: 3.02 ms +- 0.01 ms -> 3.17 ms +- 0.01 ms: 1.05x slower
- meteor_contest: 110 ms +- 2 ms -> 114 ms +- 4 ms: 1.04x slower
- pidigits: 200 ms +- 0 ms -> 208 ms +- 0 ms: 1.04x slower
- telco: 7.17 ms +- 0.21 ms -> 7.39 ms +- 0.34 ms: 1.03x slower
- html5lib: 64.0 ms +- 2.7 ms -> 65.6 ms +- 2.6 ms: 1.02x slower
- unpickle_list: 5.30 us +- 0.08 us -> 5.40 us +- 0.05 us: 1.02x slower
- pyflate: 430 ms +- 3 ms -> 438 ms +- 4 ms: 1.02x slower
- sympy_sum: 168 ms +- 2 ms -> 171 ms +- 2 ms: 1.02x slower
- xml_etree_iterparse: 110 ms +- 1 ms -> 112 ms +- 3 ms: 1.02x slower
- async_tree_memoization: 640 ms +- 25 ms -> 650 ms +- 25 ms: 1.01x slower
- chaos: 71.8 ms +- 0.7 ms -> 72.8 ms +- 0.6 ms: 1.01x slower
- hexiom: 6.71 ms +- 0.06 ms -> 6.80 ms +- 0.06 ms: 1.01x slower
- sympy_str: 299 ms +- 3 ms -> 302 ms +- 4 ms: 1.01x slower
- spectral_norm: 101 ms +- 1 ms -> 103 ms +- 1 ms: 1.01x slower
- sympy_integrate: 21.5 ms +- 0.2 ms -> 21.8 ms +- 0.2 ms: 1.01x slower
- logging_silent: 105 ns +- 1 ns -> 107 ns +- 3 ns: 1.01x slower
- raytrace: 303 ms +- 2 ms -> 306 ms +- 4 ms: 1.01x slower
- regex_v8: 23.1 ms +- 0.3 ms -> 23.3 ms +- 0.1 ms: 1.01x slower
- unpickle_pure_python: 239 us +- 2 us -> 240 us +- 2 us: 1.01x slower
- 2to3: 282 ms +- 2 ms -> 283 ms +- 1 ms: 1.00x slower

Faster (25):
- genshi_xml: 57.1 ms +- 2.7 ms -> 54.3 ms +- 1.8 ms: 1.05x faster
- scimark_sparse_mat_mult: 4.93 ms +- 0.06 ms -> 4.70 ms +- 0.10 ms: 1.05x faster
- scimark_lu: 112 ms +- 2 ms -> 109 ms +- 2 ms: 1.03x faster
- json_dumps: 13.3 ms +- 0.2 ms -> 12.9 ms +- 0.1 ms: 1.03x faster
- scimark_fft: 337 ms +- 3 ms -> 331 ms +- 3 ms: 1.02x faster
- nbody: 97.6 ms +- 1.8 ms -> 95.9 ms +- 1.8 ms: 1.02x faster
- pathlib: 23.0 ms +- 1.2 ms -> 22.6 ms +- 0.8 ms: 1.02x faster
- pickle_pure_python: 324 us +- 3 us -> 318 us +- 3 us: 1.02x faster
- genshi_text: 23.2 ms +- 0.4 ms -> 22.8 ms +- 0.3 ms: 1.02x faster
- unpack_sequence: 46.5 ns +- 0.6 ns -> 45.7 ns +- 0.7 ns: 1.02x faster
- fannkuch: 401 ms +- 6 ms -> 396 ms +- 4 ms: 1.01x faster
- chameleon: 7.06 ms +- 0.06 ms -> 6.98 ms +- 0.08 ms: 1.01x faster
- pickle_dict: 27.5 us +- 0.1 us -> 27.2 us +- 1.0 us: 1.01x faster
- pickle: 10.2 us +- 0.1 us -> 10.1 us +- 0.1 us: 1.01x faster
- xml_etree_parse: 170 ms +- 2 ms -> 168 ms +- 3 ms: 1.01x faster
- xml_etree_process: 56.8 ms +- 0.5 ms -> 56.3 ms +- 0.5 ms: 1.01x faster
- nqueens: 88.0 ms +- 1.1 ms -> 87.2 ms +- 1.1 ms: 1.01x faster
- json_loads: 26.3 us +- 0.3 us -> 26.1 us +- 0.2 us: 1.01x faster
- async_tree_none: 549 ms +- 15 ms -> 544 ms +- 12 ms: 1.01x faster
- async_tree_cpu_io_mixed: 771 ms +- 13 ms -> 766 ms +- 10 ms: 1.01x faster
- mako: 10.5 ms +- 0.0 ms -> 10.4 ms +- 0.1 ms: 1.01x faster
- crypto_pyaes: 77.2 ms +- 0.8 ms -> 76.8 ms +- 0.9 ms: 1.01x faster
- xml_etree_generate: 81.2 ms +- 0.6 ms -> 80.8 ms +- 0.5 ms: 1.00x faster
- regex_compile: 142 ms +- 1 ms -> 141 ms +- 1 ms: 1.00x faster
- scimark_sor: 121 ms +- 1 ms -> 120 ms +- 1 ms: 1.00x faster

Benchmark hidden because not significant (18): async_tree_io, deltablue, django_template, dulwich_log, float, go, logging_format, logging_simple, pickle_list, python_startup, python_startup_no_site, regex_dna, richards, scimark_monte_carlo, sqlite_synth, sympy_expand, tornado_http, unpickle

Geometric mean: 1.00x faster

thanks @ansley for doing the perf analysis on this :)

vstinner

Oh. This change reminds me my old https://peps.python.org/pep-0510/ nice :-)

Doc/c-api/function.rst

vstinner · 2022-06-16T13:49:10Z

Modules/_testcapimodule.c

+static PyObject *
+override_vectorcall(
+    PyObject *callable, PyObject *const *args, size_t nargsf, PyObject *kwnames) {
+    Py_RETURN_NONE;


Could you modify the test to return the string "overridden" instead of just returning None?

vstinner · 2022-06-16T13:53:01Z

Doc/whatsnew/3.12.rst

@@ -236,6 +236,10 @@ New Features
  an additional metaclass argument.
  (Contributed by Wenzel Jakob in :gh:`93012`.)

+* Add new function :c:func:`PyFunction_SetVectorcall` to the C API
+  which sets the vectorcall field of a given PyFunctionObject


You may explain the purpose of the function here, like mention Cinder JIT and Pyjion.

I'd rather not have a "purpose". We should state clearly what the function does, not bless some particular use case.

@adphrost
Could you add a warning that the new function pointer must have exactly the same behavior as the unaltered function.
I am a little concerned that C extensions will abuse this, and we will have deal with the bug reports.

vstinner · 2022-06-16T13:53:31Z

Misc/NEWS.d/next/C API/2022-05-03-19-35-37.gh-issue-92193.61VoFL.rst

+CPython extensions providing optimized execution of Python bytecode (like Cinder JIT and Pyjion)
+can benefit from being able to modify the vectorcall field on instances of PyFunctionObject to allow calling the optimized path (e.g. JIT-compiled) directly.
+
+This PR introduces an API call where vectorcall field can be modified like so:


This text lands into https://docs.python.org/dev/whatsnew/changelog.html#changelog which is a changlog, you should not mention "a PR" there.

I suggest to just copy/paste what you wrote in whatsnew.

vstinner · 2022-06-16T13:54:13Z

Doc/whatsnew/3.12.rst

@@ -236,6 +236,10 @@ New Features
  an additional metaclass argument.
  (Contributed by Wenzel Jakob in :gh:`93012`.)

+* Add new function :c:func:`PyFunction_SetVectorcall` to the C API
+  which sets the vectorcall field of a given PyFunctionObject


Suggested change

which sets the vectorcall field of a given PyFunctionObject

which sets the vectorcall field of a given :c:type:`PyFunctionObject`.

itamaro · 2022-06-25T23:40:46Z

@vstinner @markshannon I think all comments were addressed - any further feedback?

vstinner · 2022-06-28T22:54:55Z

Doc/c-api/function.rst

@@ -83,6 +83,12 @@ There are a few functions specific to Python functions.
   Raises :exc:`SystemError` and returns ``-1`` on failure.


+.. c:function:: void PyFunction_SetVectorcall(PyFunctionObject *func, vectorcallfunc vectorcall)
+
+   Set the vectorcall field of a given function object *func*


Suggested change

Set the vectorcall field of a given function object *func*

Set the vectorcall field of a given function object *func*.

vstinner · 2022-06-28T22:55:09Z

Doc/whatsnew/3.12.rst

@@ -236,6 +236,12 @@ New Features
  an additional metaclass argument.
  (Contributed by Wenzel Jakob in :gh:`93012`.)

+* Add new function :c:func:`PyFunction_SetVectorcall` to the C API
+  which sets the vectorcall field of a given :c:type:`PyFunctionObject`


Suggested change

which sets the vectorcall field of a given :c:type:`PyFunctionObject`

which sets the vectorcall field of a given :c:type:`PyFunctionObject`.

vstinner · 2022-06-28T22:55:51Z

Doc/whatsnew/3.12.rst

+* Add new function :c:func:`PyFunction_SetVectorcall` to the C API
+  which sets the vectorcall field of a given :c:type:`PyFunctionObject`
+  Warning: extensions using this API must preserve the behavior
+  of the unaltered function!


I don't think that this document is the right place to put a warning. Why this warning is not in the documentation of the function?

vstinner · 2022-06-28T22:56:05Z

Misc/NEWS.d/next/C API/2022-05-03-19-35-37.gh-issue-92193.61VoFL.rst

+which sets the vectorcall field of a given :c:type:`PyFunctionObject`
+
+Warning: extensions using this API must preserve the behavior
+of the unaltered function!


Please remove this warning.

vstinner · 2022-06-28T22:56:13Z

Misc/NEWS.d/next/C API/2022-05-03-19-35-37.gh-issue-92193.61VoFL.rst

@@ -0,0 +1,5 @@
+Add new function :c:func:`PyFunction_SetVectorcall` to the C API
+which sets the vectorcall field of a given :c:type:`PyFunctionObject`


Suggested change

which sets the vectorcall field of a given :c:type:`PyFunctionObject`

which sets the vectorcall field of a given :c:type:`PyFunctionObject`.

Python/ceval.c

Objects/funcobject.c

itamaro · 2022-07-27T17:54:22Z

@vstinner @markshannon thanks for the feedback. with @markshannon 's last response I think the remaining feedback is minor styling and phrasing. if that's all that left for this to be merged, I'm happy to perform these changes, but also totally ok with a core dev making these last changes when merging 😄

…(replace with assert) - func version check is sufficient

…TRIBUTE_OVERRIDDEN specialization)

itamaro · 2022-09-06T20:28:41Z

pushed a fix to LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN to deopt on function version instead of argcount

@markshannon I also reviewed the interpreter loop and didn't found any other cases that inline calls without deopting on function version. the methodology I used is searching for occurrences of inlined_py_calls - let me know if this should cover everything!

(btw, I noticed that BINARY_SUBSCR_GETITEM contains CALL_STAT_INC(inlined_py_calls) twice - is that intentional?)

Fidget-Spinner

The parts touching the specializer and specialized opcodes LGTM.

Fidget-Spinner · 2022-09-07T07:59:34Z

@itamaro

(btw, I noticed that BINARY_SUBSCR_GETITEM contains CALL_STAT_INC(inlined_py_calls) twice - is that intentional?)

No that's wrong.

markshannon

One minor formatting nit. Otherwise looks good.

Python/ceval.c

itamaro · 2022-09-07T16:30:20Z

(btw, I noticed that BINARY_SUBSCR_GETITEM contains CALL_STAT_INC(inlined_py_calls) twice - is that intentional?)

No that's wrong.

I'll file a separate issue for that

markshannon · 2022-09-07T16:59:29Z

All looks good to me.
@vstinner anything else before I merge this?

Modules/_testcapi/vectorcall.c

Objects/funcobject.c

Python/ceval.c

- PEP-7 braces - add assert - raise TypeError

…_getattribute__

Fidget-Spinner

The test LGTM in general but I have a few comments. Thanks!

Lib/test/test_call.py

1. test specialization doesn't trigger when vectorcall is already overridden 2. test specialization gets deopted when vectorcall is overridden after specialization triggered

gvanrossum · 2022-09-15T03:42:49Z

Almost ready. Who's on the hook to get this merged?

markshannon

Looks good now.

markshannon · 2022-09-15T09:14:08Z

I'll merge once the conflict has been resolved.

itamaro · 2022-09-15T15:16:28Z

I'll merge once the conflict has been resolved.

resolved!

vstinner · 2022-09-16T08:16:39Z

PyFunction_SetVectorcall() is really a cool feature, I love it! I'm not sure yet how people will manage to abuse it, but I love it :-D

Andrew Frost added 2 commits May 3, 2022 12:53

Enable setting vectorcall field on PyFunctionObjects

9cf96ec

check if vectorcall is nondefault before inlining call

282e3dc

adphrost requested a review from markshannon as a code owner May 3, 2022 19:33

bedevere-bot added the awaiting review label May 3, 2022

📜🤖 Added by blurb_it.

edcdcf1

itamaro reviewed May 3, 2022

View reviewed changes

vstinner reviewed May 3, 2022

View reviewed changes

markshannon reviewed May 3, 2022

View reviewed changes

Objects/funcobject.c Outdated Show resolved Hide resolved

Modules/_testcapimodule.c Outdated Show resolved Hide resolved

Modules/_testcapimodule.c Outdated Show resolved Hide resolved

Apply suggestions from code review

debf5a9

Change how assertion is done Co-authored-by: Itamar Ostricher <itamarost@gmail.com>

itamaro mentioned this pull request May 3, 2022

gh-92193: Check for overridden vectorcall field on PyFunctionObject #92194

Closed

addressed comments

473d18d

markshannon mentioned this pull request May 3, 2022

GH-92239: Respect PEP 523 #92245

Merged

added to docs

76e8c9d

itamaro mentioned this pull request May 25, 2022

Allow extensions to set the vectorcall field on instances of PyFunctionObject #91049

Closed

Andrew Frost added 5 commits June 15, 2022 18:05

Merge branch 'main' into pyfunctionobject-set-vectorcall-field

3337459

updated doc with fix by itamaro

1543f44

formatting

08082bd

removed doc from 3.11

ed93327

remove lines from 3.11

24ffd30

vstinner reviewed Jun 16, 2022

View reviewed changes

Apply review feedback from vstinner and markshannon

89cb4b2

vstinner reviewed Jun 28, 2022

View reviewed changes

Objects/funcobject.c Show resolved Hide resolved

itamaro added 5 commits September 6, 2022 12:58

remove redundant argcount check in LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN …

60f7769

…(replace with assert) - func version check is sufficient

Add missing periods in docs

376ee75

move warning comment to the function C API docs

56ffc70

Merge branch 'main' into pyfunctionobject-set-vectorcall-field

5340e87

fix race with pythonGH-96519 (removed func_version in LOAD_ATTR_GETAT…

a5e9d13

…TRIBUTE_OVERRIDDEN specialization)

Fidget-Spinner reviewed Sep 7, 2022

View reviewed changes

markshannon reviewed Sep 7, 2022

View reviewed changes

Python/ceval.c Outdated Show resolved Hide resolved

PEP-7 formatting

12faf52

itamaro mentioned this pull request Sep 7, 2022

BINARY_SUBSCR_GETITEM has duplicate CALL_STAT_INC(inlined_py_calls) #96653

Closed

erlend-aasland reviewed Sep 7, 2022

View reviewed changes

Modules/_testcapi/vectorcall.c Outdated Show resolved Hide resolved

Modules/_testcapi/vectorcall.c Outdated Show resolved Hide resolved

Objects/funcobject.c Show resolved Hide resolved

Python/ceval.c Show resolved Hide resolved

itamaro added 2 commits September 7, 2022 13:26

Address review feedback

6623470

- PEP-7 braces - add assert - raise TypeError

Add test for LOAD_ATTR specialization when overriding vectorcall of _…

9149f14

…_getattribute__

Fidget-Spinner reviewed Sep 8, 2022

View reviewed changes

Lib/test/test_call.py Outdated Show resolved Hide resolved

Lib/test/test_call.py Outdated Show resolved Hide resolved

Lib/test/test_call.py Outdated Show resolved Hide resolved

Improve setvectorcall + specialization testing

e732d7e

1. test specialization doesn't trigger when vectorcall is already overridden 2. test specialization gets deopted when vectorcall is overridden after specialization triggered

erlend-aasland approved these changes Sep 9, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Sep 9, 2022

markshannon approved these changes Sep 15, 2022

View reviewed changes

Merge branch 'main' into pyfunctionobject-set-vectorcall-field

84874fb

markshannon merged commit a41ed97 into python:main Sep 15, 2022

bedevere-bot removed the awaiting merge label Sep 15, 2022

kumaraditya303 mentioned this pull request Sep 17, 2022

_PyFunction_GetVersionForCurrentState should also check for overridden vectorcall field #92193

Closed


		This PR introduces an API call where vectorcall field can be modified like so:

		`void PyFunction_SetVectorcall(PyFunctionObject *func, vectorcallfunc vectorcall);`

	which sets the vectorcall field of a given PyFunctionObject
	which sets the vectorcall field of a given :c:type:`PyFunctionObject`.

	Set the vectorcall field of a given function object func
	Set the vectorcall field of a given function object func.

		@@ -0,0 +1,5 @@
		Add new function :c:func:`PyFunction_SetVectorcall` to the C API
		which sets the vectorcall field of a given :c:type:`PyFunctionObject`

gh-91049: Introduce set vectorcall field API for PyFunctionObject #92257

gh-91049: Introduce set vectorcall field API for PyFunctionObject #92257

Conversation

adphrost commented May 3, 2022 • edited by bedevere-bot

bedevere-bot commented May 3, 2022

adphrost commented May 3, 2022

itamaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markshannon left a comment

Choose a reason for hiding this comment

cpython-cla-bot bot commented May 3, 2022 • edited

itamaro commented May 4, 2022

itamaro commented May 19, 2022

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itamaro commented Jun 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itamaro commented Jul 27, 2022

itamaro commented Sep 6, 2022

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Fidget-Spinner commented Sep 7, 2022

markshannon left a comment

Choose a reason for hiding this comment

itamaro commented Sep 7, 2022

markshannon commented Sep 7, 2022 • edited

Fidget-Spinner left a comment

Choose a reason for hiding this comment

gvanrossum commented Sep 15, 2022

markshannon left a comment

Choose a reason for hiding this comment

markshannon commented Sep 15, 2022

itamaro commented Sep 15, 2022

vstinner commented Sep 16, 2022

adphrost commented May 3, 2022 •

edited by bedevere-bot

cpython-cla-bot bot commented May 3, 2022 •

edited

markshannon commented Sep 7, 2022 •

edited